Skip to content

Follow-ups from PR #21 review: canonicalise SessionStart hooks + harden tests#27

Merged
tachyon-beep merged 1 commit into
mainfrom
v1.0/10-hooks-review-followups
May 30, 2026
Merged

Follow-ups from PR #21 review: canonicalise SessionStart hooks + harden tests#27
tachyon-beep merged 1 commit into
mainfrom
v1.0/10-hooks-review-followups

Conversation

@tachyon-beep

Copy link
Copy Markdown
Collaborator

Follow-ups from the multi-agent review of #21 (merged). No behavior in #21 was a merge blocker; these address findings the review surfaced in the agent-orientation code.

Findings addressed

# Finding Fix
1/10 merge_session_start_hook short-circuited on a current entry and refreshed only the first stale one → a wrong-project pin coexisting with the current hook silently oriented the wrong project every session; multiple stale entries left survivors Canonicalise to exactly one Clarion hook running the desired command: refresh the first, remove extras (stale dup / different-project pin), drop emptied Clarion-dedicated groups. Idempotency preserved.
2 Migration 0002's in-transaction version-row INSERT was unpinned (apply_one's INSERT OR IGNORE masked its absence) migration_0002_records_its_own_version_row exercises 0002's SQL directly — verified it fails if the line is dropped. Rollback test now also asserts the version-2 row rolls back with the column.
3 install.rs non-dir-guard comment wrongly listed --skills/--hooks Corrected — only --all reaches the block (--skills/--hooks alone are Components, init_clarion()==false).
6 shell_single_quote("") (zero-iteration input) untested Added "" -> ''.
7 --force + non-directory .clarion guard untested Added install_force_rejects_non_directory_clarion.

Findings #5 (Components don't guard non-dir .clarion) is a deliberate asymmetry — those flags never touch .clarion — now documented by the #3 comment fix. #8 (non-string command silently skipped) requires hand-crafted malformed settings; left as-is (YAGNI).

Tests

TDD throughout. The two genuinely-broken merge cases failed first (the realistic different-path single-stale case already passed — confirming it was fine). The migration provenance test was verified to discriminate (fails without the in-SQL INSERT). Full gate green: fmt, clippy -D warnings, nextest (316 pass), rustdoc.

🤖 Generated with Claude Code

…tests

Follow-ups from the multi-agent review of PR #21 (merged). Three agents
converged on merge_session_start_hook under-delivering its docstring.

Behavior (review #1/#10):
- merge_session_start_hook now canonicalises Clarion-owned hooks to exactly
  one running the desired command: refresh the first, remove any extras (a
  stale duplicate, or one pinned to a different project), and drop a
  Clarion-dedicated group left empty. Previously it short-circuited on a
  current entry and refreshed only the first stale one, so a current hook
  coexisting with a wrong-project pin (hand-merged settings) silently
  oriented the wrong project every session, and multiple stale entries left
  survivors. Idempotency preserved (single exact match → no-op). Tests:
  different-path refresh, current+stale removal, multi-stale dedup.

Tests / docs:
- schema.rs: migration_0002_records_its_own_version_row exercises 0002's SQL
  directly (bypassing apply_one's INSERT OR IGNORE fallback) so the
  in-transaction version-row INSERT is load-bearing — verified it fails if
  that line is dropped. The rollback test now also asserts the version-2 row
  rolls back together with the column. (review #2)
- install.rs: correct the non-dir-guard comment — only --all reaches that
  block (--skills/--hooks alone are Components with init_clarion()==false).
  (review #3)
- install.rs tests: cover the --force non-directory .clarion guard. (review #7)
- hooks_settings.rs: shell_single_quote("") -> "''" edge case. (review #6)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 30, 2026 03:59
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@tachyon-beep
tachyon-beep merged commit 804dd93 into main May 30, 2026
3 of 4 checks passed
@tachyon-beep
tachyon-beep deleted the v1.0/10-hooks-review-followups branch June 5, 2026 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants